Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Settings] Mica & modern titlebar #25936

Merged
merged 13 commits into from
Jun 11, 2023
Merged

[Settings] Mica & modern titlebar #25936

merged 13 commits into from
Jun 11, 2023

Conversation

niels9001
Copy link
Contributor

@niels9001 niels9001 commented May 10, 2023 •
edited
Loading

Summary of the Pull Request

  • Adding Mica as a background material for Settings and OOBE windows.
  • Modern titlebar inline with W11 guidelines
  • Moving the NavigationView.PaneToggleButton to the titlebar so we have more vertical space
  • Adding a "Debug" label whenever Settings runs in debug so it's easier to distinguish when running side by side the GA version.
  • Upgraded to WinUIEx 2.2 (as it includes a theme switching fix). Moved from WinUIEx.Backdrop to Window.SystemBackdrop.

Titlebar - before vs. after
image

Mica
image

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@htcfreek
Copy link
Collaborator

@niels9001
Can you please validate that the them selection related code changes doesn't break the custom them for the Keyboard control.

@htcfreek
Copy link
Collaborator

And did you miss to push the debug label code?

@Jay-o-Way
Copy link
Collaborator

  • Can we use this?
<Window x:Class="MyApp.MainWindow">

    <Window.SystemBackdrop>
        <MicaBackdrop MicaKind="BaseAlt"/>
    </Window.SystemBackdrop>

</Window>

From https://learn.microsoft.com/nl-nl/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.media.micabackdrop?view=windows-app-sdk-1.3#examples

  • Then, Can we move away from WindowEx?

@niels9001
Copy link
Contributor Author

  • Then, Can we move away from WindowEx?

We use it for more than just Mica.. so I don't see a clear reason why we would move away from it if those helpers aren't part of WinAppSDK?

@davidegiacometti
Copy link
Collaborator

Mica & modern titlebar in settings! 😍

I don't have any problem with WinUIEx, just consider that in the 2.2 (released yesterday) backdrop APIs have been deprecated.

PowerToys is still using 2.1 at the moment.

@niels9001
Copy link
Contributor Author

Mica & modern titlebar in settings! 😍

I don't have any problem with WinUIEx, just consider that in the 2.2 (released yesterday) backdrop APIs have been deprecated.

PowerToys is still using 2.1 at the moment.

Yeah, and we'd need to update to the latest WASDK so that we can set it in XAML.. I believe there was a crash preventing this and you'd need to manually set it in code behind.

I'd let this go in as is, and then do a PR updating WinUIEx/WinAppSDK and replacing the WinUI backdrops with the WASDK backdrops.

@crutkas
Copy link
Member

crutkas commented May 11, 2023

Think we just upgraded to app sdk 1.3.1

@jaimecbernardo
Copy link
Collaborator

Yeah, main is on 1.3.1 already.

@niels9001 niels9001 marked this pull request as ready for review May 14, 2023 15:49
@niels9001
Copy link
Contributor Author

@Jay-o-Way @jaimecbernado @crutkas Updated to WinUIEx and removed the WinUIEx mica backdrops. Still using the WinUIEx Acrlyic for the flyout though, as the default DesktopAcrylicBackdrop doesn't match the required effect. Seems that this still requires code behind.

Anyways - this PR is ready for review. But maybe to wait until 0.70 is released to get this in?

@niels9001 niels9001 requested a review from jaimecbernardo May 14, 2023 15:51
@crutkas
Copy link
Member

crutkas commented May 14, 2023

I’m inclined to wait at this point.

@Jay-o-Way Jay-o-Way self-requested a review May 14, 2023 17:39
Jay-o-Way
Jay-o-Way approved these changes May 16, 2023
View reviewed changes
Copy link
Collaborator

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working and looking good! 👍🏻
Why wait?

@Jay-o-Way Jay-o-Way mentioned this pull request May 16, 2023
Closed
@jaimecbernardo
Copy link
Collaborator

I've pushed a fix for the build issue.
@niels9001 , Thanks for opening this PR. Is it ready for review?
Does any of the other utilities using WinUIEx also need changes to take the update into account? (File Locksmith, Hosts, Measure Tool, Peek, Registry Preview)

@jaimecbernardo
Copy link
Collaborator

Also, isn't the WinUIEx upgrade #26000 ? This one seems to upgrade it as well.

@niels9001
Copy link
Contributor Author

Also, isn't the WinUIEx upgrade #26000 ? This one seems to upgrade it as well.

Yeah, it was needed for this PR as well with the latest APIs and bugfixes. Feel free to merge that first.. this PR is ready for review!

@htcfreek htcfreek mentioned this pull request Jun 5, 2023
Merged
11 tasks
@jaimecbernardo
Copy link
Collaborator

Added a missing NOTICE.md entry to try to get CI to pass.
@niels9001, when comparing this PR with 0.70.1, it seems that the Settings window is no longer reacting to theme changes from dark to light and vice-versa. I mean, it seems to react, but only after navigating a bit around Settings. Any idea why this would be the case?
Other than that, it LGTM!

Jay-o-Way
Jay-o-Way suggested changes Jun 7, 2023
View reviewed changes
Copy link
Collaborator

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing

src/settings-ui/Settings.UI/App.xaml.cs Outdated Show resolved Hide resolved
@niels9001
Copy link
Contributor Author

@niels9001, when comparing this PR with 0.70.1, it seems that the Settings window is no longer reacting to theme changes from dark to light and vice-versa. I mean, it seems to react, but only after navigating a bit around Settings. Any idea why this would be the case? Other than that, it LGTM!

I have no idea.. seeing the same thing. When putting this PR in I'm 100% sure this was tested and it worked as expected 😣. When debugging, it seems that the ThemeListener.ThemeChanged event gets triggered late - so if that event would come in on time everything works as expected. So not sure how to fix it.. but maybe also not that big of a deal :)

@jaimecbernardo
Copy link
Collaborator

Alright. Anyway, theme does change eventually and it sounds like low priority. I'll open an issue after merging.
I'll do another test on Windows 10 and then I'll bring this PR in ;)
Thanks for the contribution!

jaimecbernardo
jaimecbernardo approved these changes Jun 11, 2023
View reviewed changes
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for the contribution! Works well on both Windows 10 and Windows 11.
I'll open an issue for the new theme change delay this PR seems to bring.

@jaimecbernardo jaimecbernardo merged commit 852778d into main Jun 11, 2023
@jaimecbernardo jaimecbernardo mentioned this pull request Jun 11, 2023
Closed
BLM16 pushed a commit to BLM16/PowerToys that referenced this pull request Jun 22, 2023
@niels9001 @BLM16
[Settings] Mica & modern titlebar (microsoft#25936)
648fca6 
* Adding Mica

* Working Mica

* Fluent titlebar

* Modern titlebar

* Fixing OOBE

* Fix build issue

* Add missing entry to NOTICE.md

* Update App.xaml.cs
@crutkas crutkas deleted the niels9001/modern-windowstyle branch July 20, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jay-o-Way Jay-o-Way Jay-o-Way requested changes

@jaimecbernardo jaimecbernardo jaimecbernardo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@niels9001 @htcfreek @Jay-o-Way @davidegiacometti @crutkas @jaimecbernardo @stefansjfw